Remove old, unused modals code #5068

viddo · 2015-08-17T19:02:43Z

This 🔥 🔥 🔥 code related to the now deprecated old modals.

Also removes a lot of implicit CSS, images, templates, models, views etc. that are no longer used.

Also saw some things that possibly could be removed too:

OauthController / front_layout , the only usage of old_common.css bundle, is this layout even used anymore? How can we verify that? If not, I'd suggest to 🔥 it and remove unncessary old_common styles.
fonts_ie.css.scss, really necessary anymore? Only used on public pages for some reason
is the ProximaNova still required?
considering old_common is getting phased out in favour of new styles, why not replace the submodule dir with a copy, so we can remove files easier as going forward? e.g. old_common/upload.css is no longer used
old_elements/views/dialog_base - references from inside cartodb.js makes it impossible to remove this form now, used for ui/common/share.js view, is it really required for anything outside of cartodb? If not I'd suggest to move related code inside cartodb boundary and keep cartodb.js a little bit cleaner

@xavijam can you review?
cc @CartoDB/frontend + regarding the questions listed ☝️

viddo · 2015-08-18T08:40:22Z

cc @iriberri we'll probably need to do a smoke test on this one. in theory, it's only code related to old modals, but considering the amount of removed code better to be on the safe side.

xavijam · 2015-08-18T08:41:56Z

app/assets/stylesheets/common/icons/illustration-icon.css.scss

@@ -1,6 +1,7 @@
 @import "../../variables/mixins";
 @import "../../variables/colors";
 @import "../../variables/sizes";
+@import "../../map/map-sprite";


No more sprites please, let's try to leave and they are and if we need images, create them separately.

This was migrated from app/assets/stylesheets/map/scratch_dialog.css.scss (now deleted) as it was. cc @javierarce

, let's try to leave and they are and if we need images

Not sure I understand the sentence entirely, do we leave it as it is for now or you want to change it? What would be the proper way of writing these rules?

Leave compass sprites as they are because we are still using it in the editor, but we shouldn't add more icons to them due to the fact that we will remove them when we have the new editor layout.

xavijam · 2015-08-18T08:55:39Z

I've checked that we have two modernizr files in vendor/assets/javascripts/...
Could we use just one and remove the other?

xavijam · 2015-08-18T08:58:33Z

Questions:

Do we use respond.js? I've searched through the repository and I didn't find anything.
Do we use spin.min.js? I feel, after this pull request, we are not using it anymore.

cc @CartoDB/frontend

xavijam · 2015-08-18T09:00:06Z

Seems like we forgot to remove old geocoding model, in table.js:

...
// TODO: remove when new_modals is enabled for everybody
this.geocoder = new cdb.admin.Geocoding();
...

And of course, their bindings, for example:

// Geocoder binding
this.options.geocoder.bind('geocodingComplete geocodingError geocodingCanceled', function() {
   this.notice(_t('loaded'));
}, this);
this.add_related_model(this.options.geocoder);

There are some of this type.

xavijam · 2015-08-18T09:08:14Z

In general terms, 🇺🇸, but it is difficult to evaluate the PR file by file. I encourage to pass all smoke tests related with editor view.

Is using the .min.js one so no point in having this file around

viddo · 2015-08-18T09:29:02Z

Geocoding is still used in table/header/options_menu.js, to check geocoder.isGeocoding(). I'll add it to the list of #5076 to consolidate those models too

viddo · 2015-08-18T09:36:25Z

Do we use respond.js? I've searched through the repository and I didn't find anything.

No, doesn't look like it, removing.

Do we use spin.min.js? I feel, after this pull request, we are not using it anymore.

It's still used in a handful of places (Search for new Spinner). Strangely enough, also used in cartodb.js: src/geo/ui/infowindow.js#L529, is it provided there or is some kind of implicit/hidden dependency for that view?

Conflicts: app/assets/stylesheets/common/sync_dialog.css.scss package.json

Conflicts: package.json

This reverts commit 37321a4.

@javierarce

Like in the other usages in the same view, cc @javierarce

iriberri · 2015-08-20T08:05:37Z

Ping me if you need my help.

@xavijam

Using font values from last time they were corrected, #4920. - Had to leave the type values as-is since also used for local storage keys. - Added a new attr for the iconfont. - Renamed is—new => is-new (single hyphen) to adhere to style guide. - Removed iconFont-custom rules since that type is not used (anymore) - Moved line height to the general rule since applies to all Fixes #5126 cc @xavijam @MariaCheca

Remove old, unused modals code

Nicklas Gummesson added 30 commits August 13, 2015 19:18

Remove dead code

0e95e8f

Remove old imagepicker/assets code

866149e

Remove change-title confirmation

4514100

Remove old delete-layer-confirmation

48a4412

Remove old image picker for pattern

4483c07

Remove old confirm column type confirmation

584bd09

Remove old confirm edit geom

8d244f2

Remove gmaps deprecaton confirmation

d7ebdea

Remove old delete columns confirmation

d93f96c

Remove unnecessary new_modals check

97e2faa

Remove old delete row confirmation

4d1491c

Remove old metadata code

134acbc

Remove old help modals

4e482f4

Remove ref to old geocoder working modal

36097ac

Remove old delete slide confirmation

bb9a679

Remove old delete row confirmation

44478fb

Remove old change lock view usage

e12cd45

Remove old geocoding modal usage

00e36fc

Remove old share controller usage

2a76c5e

Fix georef view replacement

60ccefe

Remove old privacy modal usage

9f85dd9

Remove old metadata modal usage

ea27941

Remove old create Layer modal usage

50e248a

Remove old basemap adder modal usage

1c20bd4

Remove old new column modal usage

a0ba94f

Remove old sync settings modal

88bfe58

Remove old sync now modal usage

3e0be88

Remove old change log modal usage

060d7a3

Remove old export modal usage

32c052e

Remove old duplicate table modal usage

e13de36

Bump frontend assets version

a2f7842

xavijam reviewed Aug 18, 2015
View reviewed changes

viddo mentioned this pull request Aug 18, 2015

Consolidate duplicated models #5076

Closed

Remove unused modernizr file

bfd1e5e

Is using the .min.js one so no point in having this file around

Nicklas Gummesson and others added 9 commits August 18, 2015 11:37

Remove unused vendor file

0096c6c

Merge remote-tracking branch 'origin/master' into 4673-rm-old-modals

ac27565

Merge remote-tracking branch 'origin/master' into 4673-rm-old-modals

75303cf

Fix using correct object

2844abf

Merge remote-tracking branch 'origin/master' into 4673-rm-old-modals

ebc8858

Conflicts: app/assets/stylesheets/common/sync_dialog.css.scss package.json

Upgrade cartodb-postgresql extension to 0.9.0

37321a4

Merge remote-tracking branch 'origin/master' into 4673-rm-old-modals

065de7f

Conflicts: package.json

Revert "Upgrade cartodb-postgresql extension to 0.9.0"

1d11cd9

This reverts commit 37321a4.

Use synchronisation object directly on table model

4c062f9

Like in the other usages in the same view, cc @javierarce

viddo mentioned this pull request Aug 20, 2015

Fix header notifications icons hell #5126

Closed

Nicklas Gummesson added 3 commits August 20, 2015 11:46

Merge remote-tracking branch 'origin/master' into 4673-rm-old-modals

553a217

Merge branch 'master' into 4673-rm-old-modals

0ea62ac

viddo added a commit that referenced this pull request Aug 24, 2015

Merge pull request #5068 from CartoDB/4673-rm-old-modals

1b7bff7

Remove old, unused modals code

viddo merged commit 1b7bff7 into master Aug 24, 2015

viddo deleted the 4673-rm-old-modals branch August 24, 2015 08:57

viddo restored the 4673-rm-old-modals branch August 24, 2015 08:57

viddo deleted the 4673-rm-old-modals branch August 24, 2015 08:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove old, unused modals code #5068

Remove old, unused modals code #5068

viddo commented Aug 17, 2015

viddo commented Aug 18, 2015

xavijam Aug 18, 2015

viddo Aug 18, 2015

xavijam Aug 18, 2015

xavijam commented Aug 18, 2015

xavijam commented Aug 18, 2015

xavijam commented Aug 18, 2015

xavijam commented Aug 18, 2015

viddo commented Aug 18, 2015

viddo commented Aug 18, 2015

iriberri commented Aug 20, 2015

Remove old, unused modals code #5068

Remove old, unused modals code #5068

Conversation

viddo commented Aug 17, 2015

viddo commented Aug 18, 2015

xavijam Aug 18, 2015

Choose a reason for hiding this comment

viddo Aug 18, 2015

Choose a reason for hiding this comment

xavijam Aug 18, 2015

Choose a reason for hiding this comment

xavijam commented Aug 18, 2015

xavijam commented Aug 18, 2015

xavijam commented Aug 18, 2015

xavijam commented Aug 18, 2015

viddo commented Aug 18, 2015

viddo commented Aug 18, 2015

iriberri commented Aug 20, 2015